refactor: replace positional tuples with readonly DTOs (AffiliationData, CriteriaData) by herpaderpaldent · Pull Request #403 · seatplus/auth

herpaderpaldent · 2026-04-28T14:41:34Z

Summary

Replaces positional tuple arrays with typed readonly DTOs throughout the role service layer.

Problem

The previous approach used positional tuple arrays like [$id, 'corporation', 'allowed'] which:

Were fragile (wrong order = silent bugs)
Provided no IDE/PHPStan type safety
Required custom validateAffiliationEntities() / validateCriteria() validators

Solution

Two new readonly DTOs:

AffiliationData — holds entity_id, entity_type, AffiliationType $affiliation_type enum
CriteriaData — holds entity_id, entity_type

Both have fromArray() factory methods for convenient construction from request data.

Changes

Add Services/Roles/DTO/AffiliationData.php and CriteriaData.php
Update AbstractRoleService::syncAffiliateManyEntities() and addCriteria() to accept variadic DTOs
Update RoleServiceInterface to match
Update AutomaticRoleService, OnRequestRoleService, OptInRoleService public methods
Update all 4 Manage*RoleAction classes
Update all tests to use DTOs
Remove validateAffiliationEntities() and validateCriteria() (DTOs + PHPStan replace them)

- Convert affiliated arrays to [entity_id, entity_type, affiliation_type] tuples before passing to syncAffiliateManyEntities(), which validates via *.0 / *.1 / *.2 numeric keys - Convert assigned arrays to [entity_id, entity_type] tuples for automaticallyAssignRoleTo() / addCriteriaForRoleApplication() / addCriteriaForRole() - Move setRoleType() before criteria writes so type-change resets (resetRoleMemberships) don't wipe newly written criteria - Add handleMembers() call after all writes to sync memberships - Remove affiliated handling from ManageManualRoleAction (manual roles have no affiliation criteria by design) - Add declare(strict_types=1) to all four action files - Update unit tests: fix mock chains (for()->andReturn($mock)), correct expected tuple arguments, add handleMembers expectations, remove now-deleted affiliated-entities test for manual action Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

what-the-diff · 2026-04-28T14:42:00Z

PR Summary

Enforced Strict Type Checking
- Added declare(strict_types=1); across all action files to increase code reliability by ensuring only the expected types of data are used.
Refined execute Function in ManageAutomaticRoleAction
- Improved handling of related entities and ensured the handleMembers() function is called after setting the role type. This enhances the efficiency and correctness of process execution.
Updated ManageManualRoleAction File
- Altered function call sequences for increased code accuracy and efficiency.
Improved ManageOnRequestRoleAction Operations
- Enhanced entity handling and ensured calls to handleMembers(), again to improve execution and accuracy.
Adjusted ManageOptInRoleAction File
- Refined operations to aggregate entity data more appropriately for more efficient data handling.
Revised Unit Test Input Structures and Assertions
- Updated input structures to include affiliation_type for related entities, thus improving test coverage. Further, enhanced assertions in unit tests to better validate the correctness of handleMembers() execution and entity data processing.
Cleaned Up Comment Lines
- Removed outdated comments, improving code readability and understanding.

…riteriaData) - Add AffiliationData and CriteriaData readonly DTOs in Services/Roles/DTO/ - Update AbstractRoleService::syncAffiliateManyEntities() and addCriteria() to accept variadic DTOs - Update RoleServiceInterface to match new signature - Update AutomaticRoleService, OnRequestRoleService, OptInRoleService to use CriteriaData variadics - Update all 4 Manage*RoleAction classes to construct DTOs from request data - Update all tests to use new DTO-based call signatures - Remove validateAffiliationEntities() and validateCriteria() validators (DTOs provide static analysis coverage) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The .phpunit.cache/ directory was already in .gitignore but got committed in the previous commit. Remove from tracking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ss() Add entityClass() method to AffiliationData and CriteriaData so callers don't need to repeat the match expression. AbstractRoleService now just calls $entity->entityClass() instead of inlining the match. Invalid entity types throw \ValueError with a descriptive message. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Replace fn (array $e) with fn (array $affiliationData) and fn (array $criteriaData) in all Manage*RoleAction classes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Manual roles support syncAffiliateManyEntities() (inherited from AbstractRoleService) for visibility scoping. The affiliated array was inadvertently removed in the DTO refactor. Also restores the 'affiliates many entities' test with withArgs() DTO-based assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ns/criteria Previously, is_truthy check on arrays meant passing affiliated: [] or assigned: [] was a no-op. Now is_array() correctly distinguishes 'key absent' (no change) from 'key present with empty array' (reset to empty). Also updates ManageAutomaticRoleActionTest to expect 0-arg calls when affiliated or assigned are provided as empty arrays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

herpaderpaldent · 2026-04-29T08:42:18Z

-            ->andReturn(['role_id' => $role->id, 'affiliated' => [['entity_id' => 1, 'entity_type' => 'corporation', 'type' => 'member']]]);
+            ->andReturn(['role_id' => $role->id, 'affiliated' => [['entity_id' => 1, 'entity_type' => 'corporation', 'affiliation_type' => 'allowed']]]);


This worries me ...
Type changed to affiliation_type
Member changed to allowed.

This is quite significant. is this still true or was the test faulty? testing something that was never matched?

herpaderpaldent · 2026-04-29T08:51:13Z

-        $mock->shouldReceive('validated')->andReturn(['role_id' => 1, 'affiliated' => [['entity_id' => 1, 'entity_type' => 'corporation', 'type' => 'member']], 'assigned' => []]);
+        $mock->shouldReceive('validated')->andReturn(['role_id' => 1, 'affiliated' => [['entity_id' => 1, 'entity_type' => 'corporation', 'affiliation_type' => 'allowed']], 'assigned' => []]);


This worries me ...
Type changed to affiliation_type
Member changed to allowed.

This is quite significant. is this still true or was the test faulty? testing something that was never matched?

paambaati/codeclimate-action@v2.6.0 used Node.js 12 which is no longer supported on GitHub Actions runners. Updated to v9 (Node.js 20) which is the current stable release. Also updated actions/checkout from v2 to v4. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

herpaderpaldent

Review response

Re: type → affiliation_type and member → allowed

The original test on origin/4.x was already wrong — testing data that would fail actual validation:

'type' => 'member' used the wrong key (type) and a nonexistent enum value (member)
RoleRequest validates affiliated.*.affiliation_type (not type)
AffiliationType has only ALLOWED, INVERSE, FORBIDDEN — there is no MEMBER case

So the original test was asserting that the action would pass through invalid data untouched — but in production, any request with type: member would fail validation before reaching the action. Our PR fixes the test to use the actual field names and valid enum values that RoleRequest enforces.

Re: CI failure — Separate issue: paambaati/codeclimate-action@v2.6.0 uses Node.js 12 which is removed from GitHub Actions runners. I've just pushed a fix updating to v9 (Node.js 20) + actions/checkout@v4. Tests should now run correctly in CI.

…avel.yml - Delete tests.yml (used paambaati/codeclimate-action@v2.6.0 which runs Node.js 12, long since removed from GitHub Actions runners) - Delete check-coding-standards.yml (ran composer update with floating deps, inconsistent with tests.yml which used composer install) - Add laravel.yml modelled on seatplus/web workflow: - concurrency group with cancel-in-progress: true to conserve resources - fail-early step order: lint → types → type-coverage → tests - XDEBUG_MODE=coverage + --coverage --min=100 enforces 100% code coverage via Pest exit code — no external service needed - actions/cache@v4 for Composer dependency caching - pgsql, pdo_pgsql, redis PHP extensions (previously missing) - actions/checkout@v4 (consistent with recent fixes) - Update README: add CI badge, expand with package overview (role types, affiliation system, SSO compliance, permission checking, dev setup) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…o 3.5.1 pest-plugin-type-coverage v3.6.1 passes [] (array) to PHPStan's RuleErrorTransformer::transform() which expects a string for $nodeType. PHPStan 1.12.25+ added strict native type enforcement that turns this into a TypeError, crashing type-coverage analysis. Pin to the same versions used by seatplus/web (phpstan 1.12.24, pest-plugin-type-coverage 3.5.1) where this bug does not occur. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The 'checks owned character ids' test was intermittently failing with 200 instead of 403. Root cause: CharacterRole rows from previous test runs (committed outside any transaction, e.g. from eveapi package tests) are visible in the current test's LazilyRefreshDatabase transaction. When CharacterInfo factory's afterCreating hook tries to save a CharacterRole with the same character_id as a stale committed row, the PK UPDATE fails, leaving the stale row (with withRandomRoles() data including 'Director') in place. The middleware then grants corporation access that should be denied. Fix: call CharacterRole::query()->delete() at the start of the test body to remove any stale rows within the transaction before the middleware runs. The transaction rollback at the end of the test restores all stale records. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

herpaderpaldent changed the title ~~fix: correct tuple format and ordering in Manage*RoleActions~~ refactor: replace positional tuples with readonly DTOs (AffiliationData, CriteriaData) Apr 29, 2026

herpaderpaldent and others added 6 commits April 29, 2026 09:47

chore: remove accidentally committed .phpunit.cache directory

1781458

The .phpunit.cache/ directory was already in .gitignore but got committed in the previous commit. Remove from tracking. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

chore: add .phpunit.cache to .gitignore

11e831b

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

style: rename abbreviated closure params $e to descriptive names

c3fbccb

Replace fn (array $e) with fn (array $affiliationData) and fn (array $criteriaData) in all Manage*RoleAction classes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

herpaderpaldent commented Apr 29, 2026

View reviewed changes

herpaderpaldent and others added 3 commits April 29, 2026 11:24

herpaderpaldent merged commit ba829e4 into 4.x Apr 29, 2026
1 check passed

herpaderpaldent deleted the fix/manage-role-actions-tuple-format branch April 29, 2026 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace positional tuples with readonly DTOs (AffiliationData, CriteriaData)#403

refactor: replace positional tuples with readonly DTOs (AffiliationData, CriteriaData)#403
herpaderpaldent merged 12 commits into
4.xfrom
fix/manage-role-actions-tuple-format

herpaderpaldent commented Apr 28, 2026 •

edited

Loading

Uh oh!

what-the-diff Bot commented Apr 28, 2026

Uh oh!

herpaderpaldent Apr 29, 2026

Uh oh!

herpaderpaldent Apr 29, 2026

Uh oh!

herpaderpaldent left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		->andReturn(['role_id' => $role->id, 'affiliated' => [['entity_id' => 1, 'entity_type' => 'corporation', 'type' => 'member']]]);
		->andReturn(['role_id' => $role->id, 'affiliated' => [['entity_id' => 1, 'entity_type' => 'corporation', 'affiliation_type' => 'allowed']]]);

		$mock->shouldReceive('validated')->andReturn(['role_id' => 1, 'affiliated' => [['entity_id' => 1, 'entity_type' => 'corporation', 'type' => 'member']], 'assigned' => []]);
		$mock->shouldReceive('validated')->andReturn(['role_id' => 1, 'affiliated' => [['entity_id' => 1, 'entity_type' => 'corporation', 'affiliation_type' => 'allowed']], 'assigned' => []]);

Conversation

herpaderpaldent commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

Changes

Uh oh!

what-the-diff Bot commented Apr 28, 2026

PR Summary

Uh oh!

herpaderpaldent Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

herpaderpaldent Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

herpaderpaldent left a comment

Choose a reason for hiding this comment

Review response

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

herpaderpaldent commented Apr 28, 2026 •

edited

Loading